-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add multi-column support to UpdateBy
RollingFormula()
operator
#6143
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review Part 1: All replicated files and files with smaller changes
Base/src/main/java/io/deephaven/base/ringbuffer/RingBuffer.java
Outdated
Show resolved
Hide resolved
Base/src/main/java/io/deephaven/base/ringbuffer/RingBuffer.java
Outdated
Show resolved
Hide resolved
...gine/table/impl/updateby/rollingformula/ringbuffervectorwrapper/RingBufferVectorWrapper.java
Outdated
Show resolved
Hide resolved
...gine/table/impl/updateby/rollingformula/ringbuffervectorwrapper/RingBufferVectorWrapper.java
Outdated
Show resolved
Hide resolved
// noinspection unchecked | ||
result = new ObjectRingBufferVectorWrapper<T>((ObjectRingBuffer<T>) buffer, (Class<T>) componentType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check to get full test coverage.
...a/io/deephaven/engine/table/impl/updateby/rollingminmax/ComparableRollingMinMaxOperator.java
Outdated
Show resolved
Hide resolved
...e/table/impl/updateby/rollingformulamulticolumn/windowconsumer/RingBufferWindowConsumer.java
Outdated
Show resolved
Hide resolved
...e/table/impl/updateby/rollingformulamulticolumn/windowconsumer/RingBufferWindowConsumer.java
Outdated
Show resolved
Hide resolved
...ble/impl/updateby/rollingformulamulticolumn/windowconsumer/CharRingBufferWindowConsumer.java
Outdated
Show resolved
Hide resolved
...e/impl/updateby/rollingformulamulticolumn/windowconsumer/ObjectRingBufferWindowConsumer.java
Outdated
Show resolved
Hide resolved
table-api/src/main/java/io/deephaven/api/updateby/UpdateByOperation.java
Outdated
Show resolved
Hide resolved
table-api/src/main/java/io/deephaven/api/updateby/UpdateByOperation.java
Outdated
Show resolved
Hide resolved
engine/table/src/test/java/io/deephaven/engine/table/impl/updateby/TestRollingFormula.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
Outdated
Show resolved
Hide resolved
table-api/src/main/java/io/deephaven/api/updateby/spec/RollingFormulaSpec.java
Outdated
Show resolved
Hide resolved
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Show resolved
Hide resolved
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Outdated
Show resolved
Hide resolved
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Outdated
Show resolved
Hide resolved
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Outdated
Show resolved
Hide resolved
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Outdated
Show resolved
Hide resolved
UpdateBy
RollingFormula()
operatorUpdateBy
RollingFormula()
operator
Base/src/main/java/io/deephaven/base/ringbuffer/RingBuffer.java
Outdated
Show resolved
Hide resolved
formulaInputSource.set(new ObjectRingBufferVectorWrapper(windowValues, inputVectorType)); | ||
// noinspection rawtypes | ||
formulaInputSource.set(new ObjectRingBufferVectorWrapper(windowValues, inputComponentType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't fully understand this change, can you please explain?
Also, can we use ByteRingBufferVectorWrapper here since we would have byte values internally? Please correct me if I am mistaken or if this is out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't fully understand this change, can you please explain?
Fixing an earlier bug. I'm trying to track the fundamental component type of the input Object column to the Vector created from it.
Also, can we use ByteRingBufferVectorWrapper here since we would have byte values internally? Please correct me if I am mistaken or if this is out of scope for this PR.
This matches AggFormula in creating ObjectVector when assembling groups of Boolean input. The functions in Basic.java
expect this input.
public abstract String formula(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change from List
to a single value require any documentation update? Also, is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepting multiple formula
was never exposed to the user so this change is entirely internal.
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Outdated
Show resolved
Hide resolved
|
||
private final String formula; | ||
private final TableDefinition tableDef; | ||
private final QueryCompilerRequestProcessor compilationProcessor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't follow where the formula
and compilationProcessor
are being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are used in RollingFormulaMultiColumnOperator#copy
, but that method is used to support PartitionedTable
proxy()
calls and can be made more efficient (by supplying the compiled formula column, eg.).
|
@@ -40,6 +40,8 @@ | |||
*/ | |||
public abstract class UpdateByOperator { | |||
protected final MatchPair pair; | |||
|
|||
// Input columns for this operator. Must be dynamic to support discovery (e.g. formula columns). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said this is OBE.
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperator.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateByOperatorFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good. We can take a more thorough/complete pass next week if needed, but I don't want to duplicate effort too much. Do want to look at things after refactoring to rely more on SelectColumn
and Selectable
.
.../engine/table/impl/updateby/rollingformulamulticolumn/RollingFormulaMultiColumnOperator.java
Outdated
Show resolved
Hide resolved
ef9aab7
to
36756da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to look at the operator code (base and multi). Deferring Python.
@@ -147,7 +155,7 @@ public boolean add(char e) { | |||
* @param count the minimum number of empty entries in the buffer after this call | |||
* @throws UnsupportedOperationException when {@code growable} is {@code false} and buffer is full | |||
*/ | |||
public void ensureRemaining(int count) { | |||
public void ensureRemaining(final int count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More opportunities to add @Override
to this code.
public void clear() { | ||
tail = head = 0; | ||
// region object-bulk-clear | ||
Arrays.fill(storage, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there are an argument for filling less, since we knew the head and tail?
/** | ||
* Create a new {@link RingBufferWindowConsumer} for the given buffer. | ||
* | ||
* @param buffer the buffer to manage and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete
/** | ||
* Push {@code count} values from the input chunk into the ring buffer, beginning at {@code index}. | ||
* | ||
* @param index the index to push the value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param index the index to push the value | |
* @param index the index of the first value to push |
* Push {@code count} values from the input chunk into the ring buffer, beginning at {@code index}. | ||
* | ||
* @param index the index to push the value | ||
* @param count the count of the value to push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param count the count of the value to push | |
* @param count the count of values to push |
* reverse window, so calling this with {@code revTicks = 1} will simply return the current row. Specifying | ||
* {@code revTicks = 10} will include the previous 9 rows to this one and this row for a total of 10 rows. | ||
* <p> | ||
* The provided {@code formula} should specify the output column name Some examples of formula are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply to all similar methods.
* The provided {@code formula} should specify the output column name Some examples of formula are: | |
* The provided {@code formula} should specify the output column name. Some examples of formula are: |
@@ -87,11 +153,4 @@ final void checkFormula() { | |||
throw new IllegalArgumentException("formula must not be empty"); | |||
} | |||
} | |||
|
|||
@Value.Check | |||
final void checkParamToken() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could do a Selectable.parse
up front to determine whether the output column name is specified when param token is missing. Maybe redundant, but that's sort of in the spirit of our immutables conventions. Could also conceivably make that selectable()
a lazily-computed, cached field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be sure you have:
- Tests for 0, 1, 2, and 3+ inputs using the new expected syntax.
- Tests that you break with >1 input if
paramToken
is supplied.
vectorColumnNameMap = new HashMap<>(); | ||
columnDefinitionMap.forEach((key, value) -> { | ||
final ColumnDefinition<?> columnDef = ColumnDefinition.fromGenericType( | ||
key, VectorFactory.forElementType(value.getDataType()).vectorType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add component type (that is, value.getDataType()
). I wonder if that means we need more testing for Object inputs.
} | ||
|
||
// Get the input column names and data types from the formula. | ||
final String[] inputColumnNames = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should validate that getColumnArrays()
returns an empty list.
No description provided.